Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Overriding desired MAC parameters and defaults #101

Merged
merged 40 commits into from
Feb 22, 2019
Merged

Overriding desired MAC parameters and defaults #101

merged 40 commits into from
Feb 22, 2019

Conversation

rvolosatovs
Copy link
Contributor

@rvolosatovs rvolosatovs commented Feb 5, 2019 •
edited
Loading

Summary:

References #5
Closes #60
Closes #104
Closes #105
Closes #144
Closes #159

Changes:

Notes for Reviewers:

Other MAC parameters, if required, will be added in another PR - please create an issue
I tested this with a Class C device in a simulator

Release notes:
Backwards-incompatible API and DB-model changes were introduced. Any created devices need to be recreated.

@rvolosatovs rvolosatovs added c/network server This is related to the Network Server compat/api This could affect API compatibility labels Feb 5, 2019
@rvolosatovs rvolosatovs added this to the February 2019 milestone Feb 5, 2019
@rvolosatovs rvolosatovs self-assigned this Feb 5, 2019
johanstokking
johanstokking requested changes Feb 5, 2019
View reviewed changes
pkg/networkserver/utils.go Outdated Show resolved Hide resolved
@rvolosatovs rvolosatovs mentioned this pull request Feb 5, 2019
Closed
@rvolosatovs
Copy link
Contributor Author

Looks like our config package does not support custom types:

panic: config: cannot work with "types.NetID" in configuration at name "ns.net_id"

goroutine 1 [running]:
go.thethings.network/lorawan-stack/pkg/config.(*Manager).setDefaults(0xc000646680, 0x15c1f8d, 0x2, 0xc0000ffb00, 0x188a820, 0xc000586d30)
        /home/rvolosatovs/src/go.thethings.network/lorawan-stack/pkg/config/config.go:485 +0x2f85
go.thethings.network/lorawan-stack/pkg/config.(*Manager).setDefaults(0xc000646680, 0x0, 0x0, 0xc0000ffb00, 0x188a040, 0xc000586800)
        /home/rvolosatovs/src/go.thethings.network/lorawan-stack/pkg/config/config.go:483 +0xaed
go.thethings.network/lorawan-stack/pkg/config.Initialize(0x1967804, 0xc, 0x195e875, 0x6, 0x188a040, 0xc000586800, 0x2bc6cd0, 0x2, 0x2, 0xc000586800)
        /home/rvolosatovs/src/go.thethings.network/lorawan-stack/pkg/config/config.go:151 +0x450
go.thethings.network/lorawan-stack/pkg/config.InitializeWithDefaults(0x1967804, 0xc, 0x195e875, 0x6, 0x188a040, 0xc000586800, 0x0, 0x0, 0x0, 0x19674e0)
        /home/rvolosatovs/src/go.thethings.network/lorawan-stack/pkg/config/config.go:188 +0x144
exit status 2

I will address this next week

johanstokking
johanstokking requested changes Feb 8, 2019
View reviewed changes
pkg/networkserver/downlink.go Outdated Show resolved Hide resolved
pkg/networkserver/grpc_gsns.go Show resolved Hide resolved
pkg/networkserver/config.go Outdated Show resolved Hide resolved
@johanstokking
Copy link
Member

@rvolosatovs

Looks like our config package does not support custom types

It does support some, indeed types.NetID should be added.

This was referenced Feb 12, 2019
Closed
Closed
htdvisser
htdvisser reviewed Feb 13, 2019
View reviewed changes
Copy link
Contributor

@htdvisser htdvisser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No specific changes requested, just a couple of comments related to API and config.

api/end_device.proto Outdated Show resolved Hide resolved
api/end_device.proto Outdated Show resolved Hide resolved
api/end_device.proto Outdated Show resolved Hide resolved
pkg/networkserver/config.go Outdated Show resolved Hide resolved
@rvolosatovs rvolosatovs added the blocking Another issue or pull request is waiting for this label Feb 14, 2019
@rvolosatovs
Copy link
Contributor Author

rvolosatovs commented Feb 14, 2019 •
edited
Loading

Blocking #104

@rvolosatovs rvolosatovs mentioned this pull request Feb 14, 2019
Closed
@rvolosatovs
Copy link
Contributor Author

@johanstokking @htdvisser any update on this?

@htdvisser
Copy link
Contributor

You didn't request my review, so I only commented on the API things that I noticed. I'll remove my review request. If you do want a full review from me, please re-request.

@htdvisser htdvisser removed their request for review February 15, 2019 07:59
@rvolosatovs rvolosatovs added ui/cli This is related to ttn-lw-cli and removed blocking Another issue or pull request is waiting for this labels Feb 15, 2019
@rvolosatovs
Copy link
Contributor Author

@htdvisser I made a bunch more changes from #144 here, since I was modifying MACSettings anyway.
Please make a full review.
Also note that I added more *Value messages in MACSettings, so there are some ugly flags in CLI again
2019-02-15-14 35 31-screenshot
Please fix that

Roman Volosatovs and others added 27 commits February 22, 2019 19:47
util: Fix config package test
5d31976
@htdvisser
cli: Add flag support for google/protobuf/wrappers
cef73e4
api: Ensure consistency of MACSettings with DeviceProfile
42add61
ns: Fix handling of joined devices
d210908
ns: Construct valid MAC state on reset
3781e78
ns: Adapt to API changes
69a419c
ns: Fix fieldmask check
56fab96
ns: Unwrap encrypted keys
8b0f172
cli: Remove redundant/non-existent device paths
c99a7e0
util: Fix NetID handling in config package
9c77fbc
ns: Fix class C downlinks
a5d4839
api: Add documentation to MACSettings fields
ed59477
ns: Fix MACSettings priority on reset
4653cf0
api,util: Make RxDelay implement encoding.Text{Unm,M}arshaler
f0c0cf2
cli: Add support for MACSettings_*Value types
3ff844c
ns: Use DevAddr from session
994bb2e
ci: Run make messages in go.lint
284ad74
dev: Regenerate messages
8c1699e
ns: Use cryptoutil.UnwrapAES128Key
a857a6f
ns: Improve readability
5b2026b
ns: Add MACSettingConfig
68e6427
ns: Make EUIs optional for ABP devices, validate for OTAA
0a8c202
ns: Fix t.Fatal usage in registry tests
cd2452a
ns: Use EndDeviceIdentifiers directly in registry tests
31faaf1
ns: Remove misplaced fields in registry tests
516193c
ns: Add additional registry create test cases
69a9b2e
ns,cli: Specify desired Rx1 delay in config
4974518
@rvolosatovs rvolosatovs merged commit a6f68ed into master Feb 22, 2019
@rvolosatovs rvolosatovs deleted the feature/ns-defaults branch February 22, 2019 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johanstokking johanstokking johanstokking approved these changes

@htdvisser htdvisser htdvisser approved these changes

Assignees

@rvolosatovs rvolosatovs

Labels
c/network server This is related to the Network Server compat/api This could affect API compatibility compat/db This could affect Database compatibility ui/cli This is related to ttn-lw-cli
Projects
None yet
Milestone
February 2019
Development

Successfully merging this pull request may close these issues.
3 participants
@rvolosatovs @johanstokking @htdvisser